Skip to content

Conversation

ZR233
Copy link

@ZR233 ZR233 commented Sep 18, 2025

No description provided.

pub use page_table_entry::MappingFlags as PageFaultFlags;

pub use crate::TrapFrame;
pub use linkme::distributed_slice as def_trap_handler;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to change these imports

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are modified by rustfmt as well 🤣

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe you can enable the CICD to check the fmt

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Starry-OS/arceos also has this problem. Since we are working on a fork and the ultimate goal is to port it back upstream, if we make a coding style change here, the diff will be a disaster.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the coding style consistent with the upstream by enabling the CI and use the same fmt confiig?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work as expected. The upstream repository does not configure rustfmt, which results in a very relaxed configuration, meaning that code formatted with the strict configuration will definitely pass checks, as is the case in this PR.

use aarch64_cpu::registers::{FAR_EL1, Readable};
use page_table_entry::MappingFlags;

fn handle_instruction_abort_lower(tf: &TrapFrame, iss: u64, is_user: bool) -> ReturnReason {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function only check whether it is a PageFault and return a ReturnReason. Maybe we can merge the check logic with

fn handle_data_abort(tf: &TrapFrame, iss: u64) {
let wnr = (iss & (1 << 6)) != 0; // WnR: Write not Read
let cm = (iss & (1 << 8)) != 0; // CM: Cache maintenance
let access_flags = if wnr & !cm {
PageFaultFlags::WRITE
} else {
PageFaultFlags::READ
};
let vaddr = va!(FAR_EL1.get() as usize);
// TODO: fixup_exception
// Only handle Translation fault and Permission fault
if !matches!(iss & 0b111100, 0b0100 | 0b1100) // IFSC or DFSC bits
|| !handle_trap!(PAGE_FAULT, vaddr, access_flags)
{
panic!(
"Unhandled EL1 Data Abort @ {:#x}, fault_vaddr={:#x}, ESR={:#x} ({:?}):\n{:#x?}\n{}",
tf.elr,
vaddr,
ESR_EL1.get(),
access_flags,
tf,
tf.backtrace()
);
}
}

  • For user mode, it returns a error reason.
  • For kernel mode, it call handle_trap directly to handle it.

@@ -1,5 +1,4 @@
.macro SAVE_REGS
sub sp, sp, {trapframe_size}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the sub code here because the RESTORE_REGS calls add code so you need to keep them consistent

ldp x6, x7, [sp, 6 * 8]
ldp x4, x5, [sp, 4 * 8]
ldp x2, x3, [sp, 2 * 8]
ldp x0, x1, [sp]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call RESTORE_REGS macro here to restore user context?

Then you can call SAVE_REGS in Task_EXIT macro.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving _enter_user and _user_trap_entry into trap.S.

Irq = 1,
Fiq = 2,
SError = 3,
Irq = 1,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add the padding here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt

pub const fn sysno(&self) -> usize {
self.r[8] as usize
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also add set_sysno

Comment on lines +84 to +85
.Ldeadloop:
b .Ldeadloop

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used at all? I suspect it's there for debugging...?

Comment on lines 19 to 26
pub esr: u64,
pub stval: usize,
}

/// Creates a new context from the given [`TrapFrame`].
pub const fn from(trap_frame: &TrapFrame) -> Self {
Self(*trap_frame)
impl ExceptionInfo {
pub fn kind(&self) -> ExceptionKind {
let esr: tock_registers::LocalRegisterCopy<u64, ESR_EL1::Register> =
tock_registers::LocalRegisterCopy::new(self.esr);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can store a LocalRegisterCopy in ExceptionInfo directly. It's Debug + Clone + Copy.

Comment on lines +81 to +91
panic!(
"Unhandled {} Instruction Abort @ {:#x}, fault_vaddr={:#x}, ESR={:#x} \
({:?}):\n{:#x?}\n{}",
if is_user { "EL0" } else { "EL1" },
tf.elr,
vaddr,
ESR_EL1.get(),
access_flags,
tf,
tf.backtrace()
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never panic for user mode exceptions. Instruct the caller that this kind of exception can not be handled.

}
}

pub fn new(entry: usize, ustack_top: VirtAddr, _arg0: usize) -> Self {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctly set arg0

impl UserContext {
pub fn run(&mut self) -> ReturnReason {
let tp_kind = unsafe { _enter_user(self) };
trace!("Returned from user space with TrapKind: {:?}", tp_kind);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

#[derive(Debug, Clone)]
pub struct UserContext {
tf: TrapFrame,
sp_el1: u64,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite familiar with AArch64, but I believe that there is no need to save SP_EL1 since SP_EL0 should be used instead in user mode? Ignore this if I'm wrong.

ldp x6, x7, [sp, 6 * 8]
ldp x4, x5, [sp, 4 * 8]
ldp x2, x3, [sp, 2 * 8]
ldp x0, x1, [sp]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving _enter_user and _user_trap_entry into trap.S.

Comment on lines +1 to +2
// AArch64 user space safe memory copy
// Based on RISC-V implementation and optimized for AArch64

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but where did this code come from? I can't find any reference, and it doesn't look like it was implemented by porting axcpu's user_copy implementation on RISC-V to AArch64...

It should account for possible page faults and set up the exception table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants